-
Notifications
You must be signed in to change notification settings - Fork 465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
db: deprecate older format versions #3152
db: deprecate older format versions #3152
Conversation
8b1266e
to
6e9f783
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great, thanks for doing this! 🚮 Just had a few small questions
Reviewed 71 of 80 files at r1, 9 of 9 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @RaduBerinde and @sumeerbhola)
format_major_version.go
line 139 at r2 (raw file):
FormatSSTableValueBlocks */ const numDeprecatedVersions = 12
nit: maybe we should keep lines within the const
stanza to increment the iota
automatically? Could do something like:
const (
// FormatMostCompatible (deprecated) ...
_ = iota
// FormatVersioned (deprecated) ...
_
open.go
line 148 at r2 (raw file):
// // To error in the first case, we set ErrorIfPristine. opts.ErrorIfNotPristine = true
I'm forgetting, will this error if there's a CURRENT
file?
compaction_iter_test.go
line 97 at r2 (raw file):
} if formatVersion < FormatDeleteSizedAndObsolete { return "testdata/compaction_iter_set_with_del"
should we keep this one for format major versions earlier than FormatDeleteSizedAndObsolete
?
db_test.go
line 79 at r2 (raw file):
func TestBasicReads(t *testing.T) { t.Skip("TODO")
still intended to be skipped?
6e9f783
to
8e0081d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR!
Reviewable status: 77 of 81 files reviewed, 4 unresolved discussions (waiting on @jbowens and @sumeerbhola)
format_major_version.go
line 139 at r2 (raw file):
Previously, jbowens (Jackson Owens) wrote…
nit: maybe we should keep lines within the
const
stanza to increment theiota
automatically? Could do something like:const ( // FormatMostCompatible (deprecated) ... _ = iota // FormatVersioned (deprecated) ... _
Good idea, done.
open.go
line 148 at r2 (raw file):
Previously, jbowens (Jackson Owens) wrote…
I'm forgetting, will this error if there's a
CURRENT
file?
No, it will error if any data operations have been performed.
compaction_iter_test.go
line 97 at r2 (raw file):
Previously, jbowens (Jackson Owens) wrote…
should we keep this one for format major versions earlier than
FormatDeleteSizedAndObsolete
?
Good catch! Fixed.
db_test.go
line 79 at r2 (raw file):
Previously, jbowens (Jackson Owens) wrote…
still intended to be skipped?
Done.
I think this is ready to merge when I get an approval. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of nits only. Thanks for doing this!
Reviewed 11 of 80 files at r1, 4 of 9 files at r2, 2 of 4 files at r3, all commit messages.
Reviewable status: 79 of 81 files reviewed, 6 unresolved discussions (waiting on @jbowens and @RaduBerinde)
format_major_version.go
line 139 at r2 (raw file):
Previously, RaduBerinde wrote…
Good idea, done.
can we add a trivial unit test to assert the actual numbers to ensure we don't renumber something by mistake?
open.go
line 147 at r3 (raw file):
// - we are retrying a failed creation. // // To error in the first case, we set ErrorIfPristine.
nit: should this say "... set ErrorIfNotPristine"?
open.go
line 1022 at r3 (raw file):
// mem is nil here. if !d.opts.ReadOnly && batchesReplayed > 0 {
curious about the addition of batchesReplayed > 0
here -- was this an existing bug?
8e0081d
to
fffa5b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR!
Reviewable status: 76 of 81 files reviewed, 4 unresolved discussions (waiting on @jbowens and @sumeerbhola)
open.go
line 1022 at r3 (raw file):
Previously, sumeerbhola wrote…
curious about the addition of
batchesReplayed > 0
here -- was this an existing bug?
I don't think it was a bug, just an unnecessary operation. I was hitting some panics in this code path while I was developing the change and adding the check helped. If I remove it now, the tests pass, but it makes sense to keep it.
This change deprecates format versions below `FormatFlushableIngest` (the 23.1 version) and sstable formats below `TableFormatPebblev1`. As part of this change, we remove code that deals with split keys; further simplifications will be made separately (e.g. removing atomic unit logic, simplifying the truncation iterator). We also remove all code related to the `CURRENT` file. The plan is to merge this after we tag a `v1.0.0` release, which will for the time being be the recommended version series for users other than CockroachDB. Informs cockroachdb#3064
fffa5b7
to
5b10d56
Compare
This test only applied to versions which are now deprecated. It is not being run since cockroachdb#3152, it should have been deleted in that PR.
This test only applied to versions which are now deprecated. It is not being run since #3152, it should have been deleted in that PR.
This change deprecates format versions below
FormatFlushableIngest
(the 23.1 version) and sstable formats below
TableFormatPebblev1
.As part of this change, we remove code that deals with split keys;
further simplifications will be made separately (e.g. removing atomic
unit logic, simplifying the truncation iterator).
We also remove all code related to the
CURRENT
file.The plan is to merge this after we tag a
v1.0.0
release, which willfor the time being be the recommended version series for users other
than CockroachDB.
Informs #3064